Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates #7112

diosmosis · 2015-01-31T05:40:10Z

This pull request modifies the tracking logic so duplicate actions will not exist in the log_action table and so even if duplicate actions exist in the log_action table, only one action ID will be used at any time.

Fixes #6436

Changes include:

new code when inserting an action, instead of just executing an INSERT and returning the last insert ID, the tracker will now:
- perform an insert
- select the smallest idaction that matches the name (another query)
- check if the last insert ID != selected ID, and if true, delete the inserted action
changes to SQL that selects idactions from the log_action table so they will always select the lowest idaction. Sometimes involves group bys.
there's an update + console command for fixing duplicates. Neither one invalidates existing archives, since I couldn't find a good way to do this in the Update w/o running possibly big queries when creating the Update SQL.
- I'm not sure if having an update is the best idea. It will automatically remove duplicates for all users, but the SQL it uses to do so is dependent on the data in the tables. So users that want to run the SQL statements themselves might be a bit confused/put-off. Also, w/o the update, I can add archive invalidation to the command.

@mnapoli @tsteur @mattab What are your thoughts?

mnapoli · 2015-01-31T07:25:22Z

I remember a discussion about a UNIQUE index, the reason it wasn't implemented that way was because of the potential index size right?

Regarding the patch I don't have a lot of feedback, I'm not familiar at all with the tracker. Is the additional query a problem? I guess it's searching against the whole table, so I wonder how it performs.

diosmosis · 2015-01-31T08:25:47Z

Do you mean the query after an INSERT? There's an index on hash + type, so it should be fast. I'm not positive about the group by's but I think the index will be used there too, since it should group by type + hash then name.

EDIT: Yeah, I was going to put a UNIQUE index, but there's no size limit on the action name, and unique indexes would require one. Since URLs can be large, a realistic limit would result in a very large index (I think).

tsteur · 2015-01-31T21:17:04Z

Do you mind writing tests for the Tracker/Model part?

I'm not positive about the group by's but I think the index will be used there too, since it should group by type + hash then name.

Maybe we can ask PRO to run such a query on a big instance with EXPLAINS to be sure an index is used?

Also not sure re the Update. I would probably just remove it and if someone wants to run it, they can use the command. Should be announced in the release blog post and mentioned in FAQ

diosmosis · 2015-02-03T16:49:19Z

@tsteur Added tests, still waiting for EXPLAINS results. Mind performing another review in the meantime?

tsteur · 2015-02-03T20:19:23Z

Looking for duplicated action ids this way looks like a pretty good solution to me 👍 It shouldn't cause much overhead as it is only done once on each action creation anyway.

The DuplicateActionRemover class contains many private methods which sometimes indicate that the class could be split into multiple classes but I'm not so deep in it as you are. Maybe It's worth it, maybe not. Might make more methods separately/easier testable when they are public afterwards. Maybe the SQL parts of that class can be also moved into the Tracker\Model part so it can be reused and easier tested when needed?

Otherwise 👍

…ays using least idaction and deleting duplicates when they are found to be inserted. Since tracker process can potentially fail before duplicates are removed, added test to make sure reports work when duplicate actions exist in the DB.

…dd time elapsed to whole command.

…group bys in Tracker/Model.php and TableLogAction.

…ion fixing command.

diosmosis · 2015-02-04T22:38:17Z

Note: I removed some private methods, but I don't think the SQL is actually useful enough to move to another class (in a plugin or in core). The queries could only be used for detecting and fixing duplicate actions.

tsteur · 2015-02-04T23:08:33Z

It's not only whether it can be reused by core or plugins. It is also that the SQL statements just rather belong to a "model" and it makes them better testable. Also you never know whether it can be maybe reused in the future. If anyone has a use case he/she would probably not find it in that class but maybe in the model... Eg those methods could be maybe useful for other cases in the future one never knows: https://github.com/piwik/piwik/pull/7112/files#diff-23c6df91cba3becc700f91001c4e39bbR203 or https://github.com/piwik/piwik/pull/7112/files#diff-23c6df91cba3becc700f91001c4e39bbR172 . At least the SQL could be extracted in another class in that plugin. Doesn't have to be core Tracker\Model I agree. This will also make the code more readable since the queries are only a detail and there would be maybe less private methods ;) There is still a lot of code in private methods which indicates that a lot of that code actually belongs somewhere else (it "indicates" it only, it doesn't mean this is always actually the case. it's up to you). It's maybe not so important here.

Can you maybe use bind params here? https://github.com/piwik/piwik/pull/7112/files#diff-23c6df91cba3becc700f91001c4e39bbR179 and here https://github.com/piwik/piwik/pull/7112/files#diff-23c6df91cba3becc700f91001c4e39bbR268 or maybe convert Ids to integer? I know we only get numbers from the database but it will prevent any possible injections in the future in case someone wants to reuse it somewhere else or so and it doesn't hurt.

…ed DuplicateActionRemover into a DAO that only deals w/ duplicate actions, created two new DAO classes (one for log_action manipulation and one for getting relational table metadata), deprecated SHOW COLUMNS method in Db in favor of new table metadata DAO, and added & fixed tests.

diosmosis · 2015-02-08T18:10:15Z

@tsteur @mnapoli I've performed a pretty big refactor on this branch, I think it merits another review. Let me know your thoughts.

mnapoli · 2015-02-08T23:10:29Z

plugins/CoreAdminHome/Model/DuplicateActionRemover.php

+            $result[$table] = $columns;
+        }
+        return $result;
+    }


since the $tablesWithIdActionColumns is hardcoded, couldn't it be the same for the columns? Or can they be dynamically added by plugins maybe?

They can be added via dimension classes (eg, ContentName). I don't know if there are any non-core ones that add idaction columns, but many core ones do so I guess it's conceivable that they exist. Also, I guess it's possible for some plugins to be disabled in which case the columns wouldn't exist.

mnapoli · 2015-02-08T23:22:03Z

plugins/CoreAdminHome/Commands/FixDuplicateLogActions.php

+        if ($logger === null) {
+            $logger = StaticContainer::get('Psr\Log\LoggerInterface');
+        }
+        $this->logger = $logger;


if you prefer those if statements could be eg:

$this->archiveInvalidator = $invalidator ?: new ArchiveInvalidator();

You could also get all those dependencies from the container so that you can use dependency injection without having null in constructors, e.g.:

class DuplicateActionRemover { // ... public function __construct(TableMetadata $tableMetadataAccess, LoggerInterface $logger)

instead of:

public function __construct($tableMetadataAccess = null, $logger = null)

You could also get all those dependencies from the container so that you can use dependency injection without having null in constructors

Would I have to access StaticContainer directly? Maybe it's possible to do it once for the console command? That would be nice.

Yes that would definitely be nice!

The problem lies with how Symfony's Console work: we need to register command objects, so that means creating every single command just to init the console (see symfony/symfony#12063).

So if we start doing dependency injection in commands, that means every dependency will be created and injected, with dependencies of dependencies, etc... So 1 or 2 command isn't a big deal but we risk running into a case where a huge load of objects (maybe most of the application) are created by the container just to end up using a few of them…

PHP-DI offers to do lazy injection but:

we can't use that in Piwik since it requires a lib that requires PHP > 5.3.20 or something

it would mean marking most services as lazy, which doesn't make any sense

this is not a clean solution anyway…

So in the meantime we are stuck with doing service location instead of dependency injection in commands…

By the way we could do something a tiny bit cleaner by introducing a get() method on commands that would proxy that to the container, e.g.:

$logger = $this->get('Psr\Log\LoggerInterface');

(like in Symfony's controllers)

That's still service location, it's just a few characters shorter (and no static calls to StaticContainer)… I don't have a strong opinion on this.

@mnapoli Is it possible to inject dependencies in-place? Ie, after an object has been created? We could maybe create commands w/o setting dependencies and then before executing a command, we resolve the dependencies and call Command::execute. The dependencies would have to be specified via the @Inject annotation I think.

Yes it's possible ($container->injectOn($obj)) but we decided not to use annotations last time it was discussed.

Another option would be to keep commands as simple as possible and move everything into classes, though that's very far from the current reality in the codebase.

Maybe a proxy command class can be used in core\Console.php and the actual command instance wouldn't be created until the proxy's execute() method is called?

Commands are instantiated because Symfony's Console needs to read the options/configuration. Even if we had a proxy commands would still be instantiated, unless we change the way commands are configured (i.e. we diverge from Symfony's official way) which would be fine by me (given this problem is pretty important)…

After thinking about it that sounds a good idea, I think I'll have a look at it (having DI only in controllers is really limiting…)

It would be awesome if it works :)

mattab · 2015-02-09T04:00:45Z

Looks good to me!

ready to merge?

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates

mattab · 2015-02-09T04:01:51Z

Well done @diosmosis

It's a real relief that this bug will be fixed! 👍

…tor.

… FixDuplicateActions.

…r changelog.

…plicates in DB when doing segment query foraction since group by in sub-query is slow.

diosmosis added 9 commits February 4, 2015 14:18

Tweak name of new test.

7d74667

Added untested command to fix duplicates in the log_action table.

31c545f

Add test for fix duplicate actions command, get command to work and a…

feda99f

…dd time elapsed to whole command.

Add update script that removes duplicate actions.

3ce72d0

Documenting new classes and commands.

31396a8

Add tests for action ID related methods in Tracker/Model.php and fix …

3e390e5

…group bys in Tracker/Model.php and TableLogAction.

Remove update and add ability to invalidate archives in duplicate act…

fd07e2f

…ion fixing command.

Remove some unneeded private methods from DuplicateActionRemover.

62af9ad

diosmosis force-pushed the 6436_action_dupes branch from 9e835c5 to 62af9ad Compare February 4, 2015 22:18

Fix test for mysql strict mode.

fc73c37

diosmosis added 6 commits February 6, 2015 09:52

Remove version change.

692129e

Merge branch 'master' into 6436_action_dupes

fa86026

Attempting to fix tests setup for mysql strict tables mode.

04629db

Fixing tests.

ebeb044

More test data insertion fixes.

c715652

Forgot to document TableMetadata DAO class.

17f681e

mnapoli reviewed Feb 8, 2015
View reviewed changes

mattab added this to the Piwik 2.11.0 milestone Feb 9, 2015

mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Feb 9, 2015

mattab changed the title ~~Fixes #6436, refacor new action insertion code so duplicate actions will not exist + provide means to fix duplicates~~ Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates Feb 9, 2015

mattab pushed a commit that referenced this pull request Feb 9, 2015

Merge pull request #7112 from piwik/6436_action_dupes

369e939

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates

mattab merged commit 369e939 into master Feb 9, 2015

diosmosis pushed a commit that referenced this pull request Feb 9, 2015

Refs #7112, do not access database in DuplicateActionRemover construc…

e084b06

…tor.

diosmosis deleted the 6436_action_dupes branch February 9, 2015 21:20

diosmosis pushed a commit that referenced this pull request Feb 9, 2015

Refs #7112, shortening constructor logic for DuplicateActionRemover &…

6ef32c1

… FixDuplicateActions.

diosmosis pushed a commit that referenced this pull request Feb 9, 2015

Refs #6436, #7112 add new duplicate action fixing command to develope…

ffb85c3

…r changelog.

diosmosis pushed a commit that referenced this pull request Feb 13, 2015

Refs #6436, #7112 fix performance regression, assume there are not du…

27bfded

…plicates in DB when doing segment query foraction since group by in sub-query is slow.

tsteur mentioned this pull request Jun 28, 2019

log_visit.referer_url and log_action.name as TEXT instead of VARCHAR cause a lot of tmp table on disk #14535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates #7112

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates #7112

diosmosis commented Jan 31, 2015

mnapoli commented Jan 31, 2015

diosmosis commented Jan 31, 2015

tsteur commented Jan 31, 2015

diosmosis commented Feb 3, 2015

tsteur commented Feb 3, 2015

diosmosis commented Feb 4, 2015

tsteur commented Feb 4, 2015

diosmosis commented Feb 8, 2015

mnapoli Feb 8, 2015

diosmosis Feb 8, 2015

mnapoli Feb 8, 2015

diosmosis Feb 8, 2015

mnapoli Feb 8, 2015

mnapoli Feb 8, 2015

diosmosis Feb 10, 2015

mnapoli Feb 10, 2015

diosmosis Feb 10, 2015

mnapoli Feb 10, 2015

mnapoli Feb 10, 2015

diosmosis Feb 10, 2015

mattab commented Feb 9, 2015

mattab commented Feb 9, 2015

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates #7112

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates #7112

Conversation

diosmosis commented Jan 31, 2015

mnapoli commented Jan 31, 2015

diosmosis commented Jan 31, 2015

tsteur commented Jan 31, 2015

diosmosis commented Feb 3, 2015

tsteur commented Feb 3, 2015

diosmosis commented Feb 4, 2015

tsteur commented Feb 4, 2015

diosmosis commented Feb 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab commented Feb 9, 2015

mattab commented Feb 9, 2015